Conversation
hiyosi
requested review from
APTy,
amartinezfayo,
azdagron,
evan2645 and
rturner3
as code owners
amartinezfayo
left a comment
amartinezfayo
left a comment
There was a problem hiding this comment.
Thank you @hiyosi for this fix and the patience for the review!
I have a small comment about the test. Looking great otherwise.
There was a problem hiding this comment.
I think that in general I prefer to have an err string here as an expected error that can be used to match with the error obtained. Test cases that don't expect errors would have an empty string. Then you can do something like
if c.err != "" {
vcs.Require().Equal(err.Error(), c.err)
} ...
in the assertions to check for the error. I think that allows us to catch some unexpected behavior if the error thrown is different than what we expect.
There was a problem hiding this comment.
Thanks for review!
Update: 5e0fd1f
amartinezfayo
left a comment
amartinezfayo
left a comment
There was a problem hiding this comment.
Thank you @hiyosi!
Could you please update the branch so we can merge this?
Signed-off-by: Tomoya Usami <tousami@zlab.co.jp>
hiyosi
force-pushed
the
fix-vault-token-auth
branch
from
5e0fd1f to
89d95c5
Compare
|
@amartinezfayo Done rebasing the branch with master branch. Thanks! |
Signed-off-by: Tomoya Usami <tousami@zlab.co.jp>
3 participants
Signed-off-by: Tomoya Usami tousami@zlab.co.jp
Pull Request check list
Affected functionality
UpstreamAuthority 'vault' plugin
Description of change
The Vault token will be set correctly in the Token Auth Method.
In v0.12.0, we get an error as below even if the token is given via plugin config or environment variable.
Which issue this PR fixes
No Issues or PRs.
https://spiffe.slack.com/archives/CBNCC2V17/p1613059475073700